Implement lint command feature #406

Daniel-Knights · 2021-04-24T19:42:43Z

As I said before, I have no experience with Crystal, so this could all be completely useless. If anything, it's a start for someone else to pick up and finish off.

This PR addresses #405.

What

Following compile.cr as an example, and considering the points raised in #405, I've made an implementation of a new command for Mint: mint lint.

How

boolean flag --json, writes the output as an array to the STDOUT
any error in mint.json is logged, but isn't captured within the JSON output
parses all files and outputs any error it finds
outputs only the first type-error it finds
exit 1 if there are any errors

Notes

It would be really useful if all type-errors could be outputted instead of only the first one, again, no idea if this is doable.

Any parse errors cause a type-error (in my test project at least) which is misleading:

Not the end of the world, but worth noting.

gdotdesign · 2021-04-25T05:09:42Z

Thanks for opening this PR! 🎉

It would be really useful if all type-errors could be outputted instead of only the first one, again, no idea if this is doable.

Any parse errors cause a type-error (in my test project at least) which is misleading:

Since parse errors leads to not including the entities in that file it might result in a type error if any entity in that file is used elsewhere. We can just not type-check if we have parsing errors.

any error in mint.json is logged, but isn't written to the JSON file (no idea if there's a way around this)

It's the same kind of error as the other so it can be catched (I'll suggest it in the review)

gdotdesign

We can skip terminal.measure calls since it doesn't make any sense to print it at the bottom.

There is a linting issue in the CI build, you can run the tests and linting with the make test command.

src/commands/lint.cr

…sure, allow for multiple type-errors

Daniel-Knights · 2021-04-25T06:49:49Z

I've added a while loop to allow for multiple type-errors to be caught, just let me know if you'd rather it didn't do this 👍

gdotdesign · 2021-04-25T06:57:43Z

I've added a while loop to allow for multiple type-errors to be caught, just let me know if you'd rather it didn't do this +1

It actually detects all type errors this way? 😮

Daniel-Knights · 2021-04-25T07:05:41Z

I've added a while loop to allow for multiple type-errors to be caught, just let me know if you'd rather it didn't do this +1

It actually detects all type errors this way? 😮

It's still limited to one type-error per file 😕, but it does capture the first for each of them

Daniel-Knights · 2021-04-25T07:59:06Z

Actually, I think it might be catching multiple per file. Perhaps it depends on the kind of type-error 🤔

It's catching type-errors from different functions in the same file, but only the first type-error from each function:

Mint - Linting
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
░ TYPE ERROR ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░

The return type of a function does not match its type definition. 

I was expecting: 

  Html

Instead it is: 

  String

Here is the relevant code snippet: 

┌──┬ source/components/Exchange.mint:38:3 ─────────────────────────────────────┐
│34│       }                                                                   │
│35│     }                                                                     │
│36│   }                                                                       │
│37│                                                                           │
│38│   fun render : Html {                                                     │
│39│                                                                           │
│40│     ""                                                                    │
│41│   }                                                                       │
│42│ }                                                                         │
└──┴───────────────────────────────────────────────────────────────────────────┘


░ TYPE ERROR ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░

You were trying to assign an incompatible value to the status state. 

The type of the state is: String 

But the type you are trying to assign to it: Number 

Here is where you did the assignment: 

┌──┬ source/components/Exchange.mint:23:12 ────────────────────────────────────┐
│19│     next { baseValues = values }                                          │
│20│   }                                                                       │
│21│                                                                           │
│22│   fun updateCompareLocale (values : InputValues) : Promise(Never, Void) { │
│23│     next { compareLocale = 6 }                                            │
│24│   }                                                                       │
│25│                                                                           │
│26│   fun handleSubmit (e : Html.Event) {                                     │
│27│     sequence {                                                            │
└──┴───────────────────────────────────────────────────────────────────────────┘

And here is where the state is defined: 

┌──┬ source/components/Exchange.mint:16:3 ─────────────────────────────────────┐
│12│     display: flex;                                                        │
│13│   }                                                                       │
│14│                                                                           │
│15│   state baseValues : InputValues = InputValues("0", "GBP")                │
│16│   state compareLocale : String = "USD"                                    │
│17│                                                                           │
│18│   fun updateBaseValues (values : InputValues) : Promise(Never, Void) {    │
│19│     next { baseValues = values }                                          │
│20│   }                                                                       │
└──┴───────────────────────────────────────────────────────────────────────────┘


░ TYPE ERROR ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░

The 1st argument to a function is causing a mismatch. 

The function is expecting the 1st argument to be: 

  Html.Event(
    bubbles: Bool,
    ...
    event: Html.NativeEvent)

Instead it is: 

  String

The type of the function is: 

  Function(
    Html.Event(
      bubbles: Bool,
      ...
      event: Html.NativeEvent),
    Void)

You tried to call it here: 

┌──┬ source/components/Exchange.mint:28:32 ────────────────────────────────────┐
│24│   }                                                                       │
│25│                                                                           │
│26│   fun handleSubmit (e : Html.Event) {                                     │
│27│     sequence {                                                            │
│28│       Html.Event.preventDefault("e")                                      │
│29│                                                                           │
│30│       if (baseValues.amount == "0") {                                     │
│31│         setStatusMessage(6)                                               │
│32│       } else {                                                            │
└──┴───────────────────────────────────────────────────────────────────────────┘

Daniel-Knights · 2021-04-25T08:05:30Z

I'm also wondering if it's worth formatting the JSON better. So, have each error as an object with separate properties, one for error type, one for line/character number and one for message. I'm not sure there's a simple way to do that, but, just a thought

src/commands/lint.cr

gdotdesign

I just found one issue which is basically the exceptions are printed to STDOUT even if the json flag is set, if fixed then it's good to go for me 👍

src/commands/lint.cr

gdotdesign · 2021-04-26T07:34:29Z

I'm also wondering if it's worth formatting the JSON better. So, have each error as an object with separate properties, one for error type, one for line/character number and one for message. I'm not sure there's a simple way to do that, but, just a thought

Yeah, it would be great if the errors would contain the source path, line and column numbers but it's a bit tricky to do now, so let's add that as an enchantment for the future.

src/commands/lint.cr

Dan Knights added 2 commits April 24, 2021 20:07

lint command first implementation

8068edf

remove unnecessary runtime var

2f4af10

Sija added language Language feature tooling Tooling related feature (formatter, documentation, production builder) enhancement New feature or request labels Apr 24, 2021

Sija requested a review from gdotdesign April 24, 2021 22:46

gdotdesign reviewed Apr 25, 2021

View reviewed changes

src/commands/lint.cr Outdated Show resolved Hide resolved

src/commands/lint.cr Outdated Show resolved Hide resolved

Dan Knights added 2 commits April 25, 2021 07:44

lint: update description, catch mint.json errors, remove terminal.mea…

fa75330

…sure, allow for multiple type-errors

move assignment to the top

a564d14

Sija requested changes Apr 25, 2021

View reviewed changes

Dan Knights added 2 commits April 25, 2021 18:43

output to stdout instead

ebf032e

exit ternary

b18d1e1

gdotdesign requested changes Apr 26, 2021

View reviewed changes

src/commands/lint.cr Show resolved Hide resolved

src/commands/lint.cr Outdated Show resolved Hide resolved

Dan Knights added 2 commits April 26, 2021 09:30

print all errors

c218060

print errors with loop

9cc36f2

gdotdesign approved these changes Apr 26, 2021

View reviewed changes

Sija requested changes Apr 26, 2021

View reviewed changes

src/commands/lint.cr Outdated Show resolved Hide resolved

Update src/commands/lint.cr

890c231

Sija reviewed Apr 26, 2021

View reviewed changes

src/commands/lint.cr Outdated Show resolved Hide resolved

Update src/commands/lint.cr

540a468

Sija reviewed Apr 26, 2021

View reviewed changes

src/commands/lint.cr Outdated Show resolved Hide resolved

Update src/commands/lint.cr

4555561

Sija linked an issue Apr 26, 2021 that may be closed by this pull request

Feature request: mint lint command #405

Closed

gdotdesign merged commit 538dc78 into mint-lang:master Apr 26, 2021

Sija added a commit that referenced this pull request Apr 26, 2021

Followup to #406

5c5caff

Sija added this to the 0.13.0 milestone Jun 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement lint command feature #406

Implement lint command feature #406

Daniel-Knights commented Apr 24, 2021 •

edited by Sija

gdotdesign commented Apr 25, 2021

gdotdesign left a comment •

edited

Daniel-Knights commented Apr 25, 2021

gdotdesign commented Apr 25, 2021

Daniel-Knights commented Apr 25, 2021

Daniel-Knights commented Apr 25, 2021 •

edited

Daniel-Knights commented Apr 25, 2021 •

edited

gdotdesign left a comment

gdotdesign commented Apr 26, 2021

Implement lint command feature #406

Implement lint command feature #406

Conversation

Daniel-Knights commented Apr 24, 2021 • edited by Sija

What

How

Notes

gdotdesign commented Apr 25, 2021

gdotdesign left a comment • edited

Choose a reason for hiding this comment

Daniel-Knights commented Apr 25, 2021

gdotdesign commented Apr 25, 2021

Daniel-Knights commented Apr 25, 2021

Daniel-Knights commented Apr 25, 2021 • edited

Daniel-Knights commented Apr 25, 2021 • edited

gdotdesign left a comment

Choose a reason for hiding this comment

gdotdesign commented Apr 26, 2021

Daniel-Knights commented Apr 24, 2021 •

edited by Sija

gdotdesign left a comment •

edited

Daniel-Knights commented Apr 25, 2021 •

edited

Daniel-Knights commented Apr 25, 2021 •

edited